Add atomic Content database row migrations - #2573
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Visual recap — screenshot failedA recap was published, but the PR-comment screenshot could not be captured or uploaded. Open the interactive recap directly: Open the full interactive recap Diagnostic: light: page.screenshot: Timeout 30000ms exceeded. Call log: - taking page screenshot - waiting for fonts to load... |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…database-migration # Conflicts: # templates/content/actions/delete-database-items.ts # templates/content/parity/matrix.md # templates/content/parity/matrix.ts
…database-migration # Conflicts: # package.json
…database-migration
This comment has been minimized.
This comment has been minimized.
…database-migration
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🔴
Review Details
Incremental Code Review Summary
The existing restore-versus-migration issue remains unresolved and was not reposted. The latest changes add broader trash/permanent-delete lock helpers and extensive lifecycle race coverage, but the permanent-delete path still has pre-lock collection windows that can invalidate the lock set and deletion inputs.
This remains high risk because recursive deletion mutates database memberships, documents, and receipt-bearing state across multiple databases. Two new high-severity findings were identified:
- 🔴 HIGH — Permanent deletion can discover external memberships before locking, then fail to lock a database attached during the gap.
- 🔴 HIGH — Permanent deletion can use a stale pre-lock subtree, delete a newly added membership/database record, and leave the newly created row document orphaned.
🧪 Browser testing: Skipped — PR changes backend actions, persistence, tests, CI, parity metadata, and documentation; no user-facing UI implementation changed.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🔴
Review Details
Incremental Code Review Summary
The prior restore-versus-migration issue is fixed: restore now collects its scope, acquires database and membership locks, rechecks scope coverage, and only then restores rows. That thread was resolved. The two prior permanent-deletion lock/rebuild comments remain open and were not reposted.
The latest lifecycle changes are otherwise well covered by SQLite/PGlite/PostgreSQL tests. One new high-severity race remains in permanent deletion: a trashed subtree is collected before waiting for its locks, and the Trash state is not revalidated after the locks are acquired.
New finding
- 🔴 HIGH — A concurrent restore can complete while permanent deletion waits for the database lock; deletion then proceeds with the stale document set and deletes the restored subtree because cleanup lacks a Trash-state predicate.
🧪 Browser testing: Skipped — PR changes backend actions, persistence, tests, CI, parity metadata, and documentation; no user-facing UI implementation changed.
…database-migration # Conflicts: # templates/content/actions/configure-document-property.ts # templates/content/actions/delete-content-database.ts # templates/content/actions/delete-document-property.ts # templates/content/actions/delete-document.ts # templates/content/actions/duplicate-database-item.ts # templates/content/actions/duplicate-database-items.ts # templates/content/actions/permanently-delete-document.ts # templates/content/actions/set-document-property.ts # templates/content/parity/matrix.md # templates/content/server/plugins/db.ts
Problem
Content exposes reliable single-row edits, but a bounded reorganization that must update every existing row body and schema cannot safely loop those actions. A timeout or retry could otherwise leave a database partially migrated, with no durable way to distinguish an incomplete write from a completed response that was lost.
The migration also has to coordinate with ordinary Content lifecycle writers. Without one lock order and post-lock revalidation, property/schema changes, trash, restore, and permanent deletion can race the migration or act on a stale deletion scope.
Approach
Add one Content-specific action for whole-database row migrations. It validates the exact active row set, applies one transaction, requires separate readback verification before destructive finalization, and keeps rollback and terminal retries receipt-backed and drift-guarded.
Participating Content lifecycle operations use database → sorted membership lock order. Permanent deletion now rebuilds and revalidates its full database-and-membership scope after acquiring those locks; if a concurrent membership expands the ordered lock set, the entire transaction retries from a fresh snapshot.
This PR deliberately does not add a framework-generic bulk writer, repair MCP authentication, implement a shared document-lifecycle protocol for every create/reparent writer, deploy anything, or migrate any real Content database.
What changed
migrate-content-database-rowswithvalidate,apply,verify,rollback, andfinalizephases.applyingclaim before row mutation.Safety and operations
The action accepts ordinary databases without attached Sources and is capped at 100 rows, properties, options, and protected values. Database admin and per-row access checks are required. Content, identity, ownership, personal-space metadata, privacy, shares, property descriptions, source mappings, and values participate in semantic drift checks.
Rollback is available only while the guarded post-apply state still matches. Finalization removes only receipt-recorded legacy properties after the separate verified state. The schema migration is additive. Remote shared SQLite-family backends reject new mutations before any editor flush; existing terminal receipts remain replayable no-ops.
No production or private Content data was read or changed. All test documents, PostgreSQL clusters, PGlite installs, and temporary driver links were synthetic, bounded, cleaned up, and read back as absent.
Verification
Exact head:
9082af762pnpm test:content-db: 13 files and 200 tests passed across the complete migration action suite on mergedmain.pnpm --filter content typecheckandpnpm fmt:check: passed.pnpm test:content-product-impact: 30 tests passed.pnpm guards: all 42 checks passed on the sealed merge commit.pnpm oxlint: passed with repository baseline warnings only.skippedon this merge head and did not publish a re-runnable check. Fresh independent technical review therefore remains pending; the older approval is not represented here as exact-head review.Review focus
Follow-up
S2573-F1, the shared document-lifecycle protocol across create, membership, and reparent writers, remains a separate shaped workstream. Its preserved experiment is not part of this branch and stays on hold pending the Content implementation-versus-roadmap audit.